docs(architecture): adopt tracking-number identifiers, and add agent skills - #93
Merged
Merged
Conversation
…skills This repository had no architecture decision records, so there is nothing to migrate: it starts on the convention instead of adopting it after a collision. The model matches the rest of the ecosystem (exelearning/exelearning#2232). Identifiers come from the GitHub tracking number of the change — its issue when it has one, otherwise its pull request. GitHub draws both from one repository-wide sequence, so they never collide. Issues are disabled here (`has_issues: false`), so in practice every number is a PR number, and no issue can or should be opened just to obtain one. docs/architecture/adr/ADR-<number>-<NN>-<decision-slug>.md docs/architecture/changes/<number>-<change-slug>/ There is no committed index. `make architecture-records` prints it from frontmatter; a generated file in git conflicts on every concurrent branch, so `check` rejects one if it ever appears. Tooling is `tools/architecture_records.py`, matching the existing `tools/*.py` convention, standard library only — no YAML dependency for a documentation linter. Its own tests use `unittest`, so they add no dependency either. Also adds agent guidance, following the layout used by the sibling repos: `CLAUDE.md` pointing at `AGENTS.md`, `.agents/skills/` with the skills, and `.claude/skills/` symlinking to them per skill. architecture-records the convention and what the validator enforces nextcloud-app-development lib/ structure, DI, routes, preview provider elpx-package-safety entry-path normalization as the security boundary testing the Vitest/PHPUnit split and what it cannot prove verify, release the local pipelines security-audit ported unchanged from the sibling repos None of this ships: `tools`, `docs`, `.agents`, `.claude`, `AGENTS.md` and `CLAUDE.md` are excluded in both `.distignore` and `.gitattributes`. Verified by staging the package with the same rsync `make package` uses — the result contains only appinfo, blueprint.json, img, lib, LICENSE, README.md, src and templates. make architecture-check OK, 0 ADRs, 0 changes make architecture-test OK, 35 tests npm run typecheck exit 0 npm test exit 0, 6 files / 59 tests make lint exit 0
Contributor
Preview this PR in the Nextcloud Playground
A fresh Nextcloud boots in your browser with this branch's eXeLearning editor: |
The first record under the new convention, and a demonstration of it: the tracking number is this pull request, because issues are disabled on this repository. It documents the decision, the three rejected alternatives, and the evidence — including that issues are disabled on all four satellite repositories, which makes any issue-mandating rule unimplementable across the ecosystem. The validator rejected the first draft for naming the retired four-digit form in its own prose. Reworded rather than allowlisted: the rule should not need an exception for the record that introduces it. make architecture-check OK, 1 ADRs, 0 changes make architecture-test OK, 35 tests
…ed script Four repositories had four implementations of the same rules — roughly 3,800 lines of validator and 2,700 of tests, in two languages — and they had already started to diverge. A differential test over the same adversarial fixtures found the PHP validators accepting a record with no `deciders` that the Python one rejected. There is now one implementation, byte-identical everywhere: architecture_records.py one file, standard library only architecture_records_test.py 48 tests, unittest, no dependency Python because it is the only runtime guaranteed on every CI image here, and because two of the four were already written in it. Only paths and the record prefix are configurable, in `architecture-records.json`. Every rule and every frontmatter key is identical in every repository, so the divergence cannot come back. Two things had to be reconciled to get there: - `deciders` is optional. Two repositories legitimately have records without it; inventing authorship would be worse than not recording it. - `related.prs` holds bare integers of this repository; cross-repository references move to `external_refs` as full URLs, which is what the omeka policy already said and its validator did not enforce. `0` is added as a tracking-number sentinel for records that predate tracking. GitHub numbers issues and pull requests from 1, so 0 can never collide with a real one.
…on port The validator is now one TypeScript file, byte-identical in every repository. It runs under Bun in core, where `bun test` covers it, and under the Node that ships on the CI image in the plugins, which need no setup step and no dependency. This replaces the Python port from the previous commit. Python was chosen when the alternative looked like installing Bun in four PHP pipelines. It is not: `setup-bun` appears only in release workflows that never run on a pull request, but Node is preinstalled on the runner image, and one source file runs under both runtimes once it avoids Bun-specific APIs. That keeps the tests where the scaffolding already is. Core keeps its 47 `bun test` cases with `expect()`; the plugins run the file itself on every pull request, which is what would surface a Node-specific regression. Repository-specific values live in `architecture-records.json` — paths and the record prefix only. Every rule is identical everywhere. bun run scripts/architecture-records.mts check OK node scripts/architecture-records.mts check OK (same output)
…sing Making `deciders` optional was the wrong reconciliation. It weakened the rule in every repository to accommodate five records that simply had not recorded who decided — a gap to fill, not a rule to relax. The field is required again, and the five records in wp-exelearning and omeka-s-exelearning now carry `@erseco` and `claude-code`, matching the pattern moodle-mod_exelearning already used for the same work. No model had to be inferred: `ai_assistance` was already present and consistent with each record's date (claude-fable-5 in July, claude-opus-5 in August). wp, omeka, nextcloud, moodle, core architecture-check OK core bun test 47 pass
Five documents still pointed at `scripts/architecture-records.ts`, which no longer exists: the file became `.mts` so the same source runs under Bun in core and under Node in the plugin repositories. The script's own usage header said `bun run` unconditionally, which is wrong in the four repositories that carry a copy and run it with Node. It now states both, and why the file must avoid runtime-specific APIs. core architecture-check OK, bun test 47 pass, lint OK all 5 byte-identical copies, architecture-check OK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
This repository has no architecture decision records, so there is nothing to migrate. It starts on the convention rather than adopting it after a collision — which is the cheapest moment to do it.
The model matches the rest of the ecosystem:
exelearning/exelearning#2232.Identification
Records are named after the GitHub tracking number of the change they belong to — its issue when there is one, otherwise its pull request. GitHub allocates issue and PR numbers from a single repository-wide sequence, so the two can never collide.
Issues are disabled on this repository:
So every number here is a PR number, and no issue can — or should — be opened merely to obtain one.
<NN>is a two-digit sequence scoped to that number alone, starting at01, present even for a single ADR so that adding a second one later never renames the first. There is no global counter — that rule is unsafe on parallel branches, because the collision lands in the filename and Git merges both files cleanly without reporting anything.The index is not a file
make architecture-recordsprints it from frontmatter. Committing it would guarantee a merge conflict on every concurrent branch, sochecktreats its presence as an error.Tooling
tools/architecture_records.py, matching the existingtools/*.pyconvention. Standard library only — no YAML dependency for a documentation linter, and its own tests useunittest, so they add no dependency either.make lintnow depends onarchitecture-check, and CI gets its ownarchitecture-recordsjob that also prints the index into the run summary.It validates: filename grammar (rejecting the retired global form) ·
id↔ filename ·tracking_issue↔ filename/directory · H1 ↔ frontmatter · duplicate ids and sequences · status vocabularies · calendar dates · resolvable ADR/change references · numeric issue/PR values ·supersedes/superseded_bysymmetry and theSupersededstatus · retired identifiers anywhere in the tree · a committed index.Agent guidance
Following the layout the sibling repos use:
CLAUDE.mdpointing atAGENTS.md,.agents/skills/holding the skills, and.claude/skills/symlinking to them per skill.architecture-recordsnextcloud-app-developmentlib/structure, constructor injection, routes, the preview provider, what must stay off the serverelpx-package-safetytestingverify,releasesecurity-auditAGENTS.mdgains an architecture-records section and a skills index.Nothing here ships
tools,docs,.agents,.claude,AGENTS.mdandCLAUDE.mdare excluded in both.distignoreand.gitattributes.Verified rather than assumed — staged the package with the same rsync invocation
make packageuses:No
tools, nodocs, no.agents, no.claude, noAGENTS.md, noCLAUDE.md, and no file matching*architecture*.Commands run
make architecture-checkmake architecture-testnpm run typechecknpm testmake lintvendor/bin/phpunitwas not run: no PHP dependencies are installed in this environment. This change touches no PHP.A finding, reported not fixed
While writing
elpx-package-safetyI compared the three entry-path normalizers. They do not agree, and the TS docblock claims they do.ZipEntryServicepaths.ts/ SW../escapenullnulla/b/../cnulla/ca/./bnulla/ba//ba//ba/bNot a traversal hole — both reject
../escape, neither escapes the package root. It is a consistency defect: a package containinga/b/../crenders in the browser but 404s from the PHP asset controller and the preview provider. The comment onnormalizeEntryPathsaying it "matches the rule used by the PHP-sideZipEntryService" is simply false, which is the part most likely to mislead.Converging them changes behaviour at a security boundary, so it belongs in its own PR with its own ADR. Documented in the skill; not fixed here.
Follow-up